Skip to content

Migrate from Query::single and friends to Single #15872

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
alice-i-cecile merged 11 commits into from
Oct 13, 2024
Merged

Migrate from Query::single and friends to Single #15872

alice-i-cecile merged 11 commits into from
Oct 13, 2024

Conversation

@pablo-lua
Copy link
Contributor

@pablo-lua pablo-lua commented Oct 12, 2024

Objective

Solution

  • Simply migrate where possible.

Testing

  • Expect that CI will do most of the work. Examples is another way of testing this, as most of the work is in that area.

Notes

For now, this PR doesn't migrate QueryState::single and friends as for now, this look like another issue. So for example, QueryBuilders that used single or World::query that used single wasn't migrated. If there is a easy way to migrate those, please let me know.

Most of the uses of Query::single were removed, the only other uses that I found was related to tests of said methods, so will probably be removed when we remove Query::single.

@pablo-lua pablo-lua added C-Examples An addition or correction to our examples C-Code-Quality A section of code that is hard to understand or change A-Cross-Cutting Impacts the entire engine S-Needs-Review Needs reviewer attention (from anyone!) to move forward D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Oct 12, 2024
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Oct 13, 2024

I've had to open this up in Codespaces to review it, as Github's webpage hates PRs with many files changed 🙃 That said, looks great! I particularly appreciate the use of Option<Single> in tests to ensure that we're still panicking if things aren't found.

@rparrett
Copy link
Contributor

rparrett commented Oct 13, 2024 •
edited
Loading

Guessing this is going to have conflicts after #15857. Can take a look after those get fixed up.

@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Oct 13, 2024
BenjaminBrienen
BenjaminBrienen approved these changes Oct 13, 2024
View reviewed changes
@pablo-lua
Copy link
Contributor Author

pablo-lua commented Oct 13, 2024

@BenjaminBrienen All comments addressed.

@pablo-lua pablo-lua added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 13, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 13, 2024
Merged via the queue into bevyengine:main with commit d96a9d1 Oct 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@bas-ie bas-ie Awaiting requested review from bas-ie

@rparrett rparrett Awaiting requested review from rparrett

+1 more reviewer

@BenjaminBrienen BenjaminBrienen BenjaminBrienen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-Cross-Cutting Impacts the entire engine C-Code-Quality A section of code that is hard to understand or change C-Examples An addition or correction to our examples D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Milestone

0.15

Development

Successfully merging this pull request may close these issues.

Replace all uses of Query::single with Single

4 participants

@pablo-lua @alice-i-cecile @rparrett @BenjaminBrienen